@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Yingwei Zheng (dtcxzyw)

diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index ee5874b26f534..075f3f435ad74 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -1883,8 +1883,10 @@ llvm::Constant *ConstantEmitter::tryEmitPrivateForVarInit(const VarDecl &D) {
 
   // Try to emit the initializer.  Note that this can allow some things that
   // are not allowed by tryEmitPrivateForMemory alone.
-  if (APValue *value = D.evaluateValue())
-    return tryEmitPrivateForMemory(*value, destType);
+  if (APValue *value = D.evaluateValue()) {
+    if (!value->allowConstexprUnknown())
+      return tryEmitPrivateForMemory(*value, destType);
+  }
 
   return nullptr;
 }
diff --git a/clang/test/CodeGenCXX/cxx23-p2280r4.cpp b/clang/test/CodeGenCXX/cxx23-p2280r4.cpp
new file mode 100644
index 0000000000000..d5409be451df0
--- /dev/null
+++ b/clang/test/CodeGenCXX/cxx23-p2280r4.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++23 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++17 %s -emit-llvm -o - | FileCheck %s
+
+extern int& s;
+
+// CHECK: @_Z4testv()
+// CHECK-NEXT: entry:
+// CHECK-NEXT: [[I:%.*]] = alloca ptr, align {{.*}}
+// CHECK-NEXT: [[X:%.*]] = load ptr, ptr @s, align {{.*}}
+// CHECK-NEXT: store ptr [[X]], ptr [[I]], align {{.*}}
+int& test() {
+  auto &i = s;
+  return i;
+}

Does it even make sense for evaluateValue to return a constexpr-unknown value? If I'm following correctly, we can't actually do anything useful with such a result: we don't actually know what the value is. It would make things simpler if we kept such APValues as an internal implementation detail of ExprConstant.

Clang rejects the following case after modifying evaluateValue to treat constexpr-unknown value as invalid: https://godbolt.org/z/5K9vvnscT

struct Bottom { constexpr Bottom() {} };
struct Base : Bottom {
  constexpr Base(int a = 42, const char *b = "test") : a(a), b(b) {}
  int a;
  const char *b;
};
struct Base2 : Bottom {
  constexpr Base2(const int &r) : r(r) {}
  int q = 123;
  const int &r;
};
struct Derived : Base, Base2 {
  constexpr Derived() : Base(76), Base2(a) {}
  int c = r + b[1];
};

constexpr Derived derived;
constexpr Bottom &bot2 = (Base2&)derived;
constexpr Base2 &ok2 = (Base2&)bot2;

File /home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/clang/test/SemaCXX/constant-expression-cxx11.cpp Line 936: constexpr variable 'ok2' must be initialized by a constant expression

Clang rejects the following case after modifying evaluateValue to treat constexpr-unknown value as invalid

I don't see the connection. At least, there isn't any obvious connection: the testcase doesn't require constexpr-unknown. Maybe you need to "reject" the value earlier?

Clang rejects the following case after modifying evaluateValue to treat constexpr-unknown value as invalid

I don't see the connection. At least, there isn't any obvious connection: the testcase doesn't require constexpr-unknown. Maybe you need to "reject" the value earlier?

See #128409. I have made some attempts but still cannot fix the test failure.
BTW evaluateValue is also used by evaluateVarDeclInit so that we cannot propagate constexpr-unknown flags during constexpr evaluation.

How do people feel about landing and backporting this as a stop-gap solution for clang 20 while we investigate whether a cleaner design can be found in trunk?

I'm worried this is just scratching the surface... it looks like there's a whole class of issues involving constexpr-unknown leaking out of constant evaluation, and this is just addressing one specific case. If we can't take #128409 on the branch, we should just disable the feature and give it more time to bake on main.

Can I suggest adding this to the 20.X milestone to bring more attention to it so it goes through before the release? clang is quite unusable at the moment because of this

I'm worried this is just scratching the surface... it looks like there's a whole class of issues involving constexpr-unknown leaking out of constant evaluation, and this is just addressing one specific case. If we can't take #128409 on the branch, we should just disable the feature and give it more time to bake on main.

I think it would be a shame to lose the feature, it is incredibly useful. I looked at #128409 and don't have any problems with it, we could definitely cherry pick it.

That said, this patch might be worth having in that branch AS WELL to cover any other missed cases. I would be OK having this patch HERE only on that branch or replaced with an assert in main.

Closed in favor of #128409

That said, this patch might be worth having in that branch AS WELL to cover any other missed cases. I would be OK having this patch HERE only on that branch or replaced with an assert in main.

#128409 should cover all the cases this patch would cover. I'm a little concerned #128409 still isn't broad enough... but not in a way that would trigger this check. I'll continue discussion there.